quic: reuse TLS pause machinery to drop event deferral & improve 0RTT - #65522
quic: reuse TLS pause machinery to drop event deferral & improve 0RTT#65522pimterry wants to merge 5 commits into
Conversation
With this change, the TLS pause mechanisms used in node:tls are now made generic, and used in QUIC to pause the handshake entirely for the session event, thereby stopping almost all events being emitted (everything except qlog) so that all the previously existing deferral machinery is unnecessary. This is useful immediately to clean up & simplify things, but also medium term because this makes the 0RTT flow significantly clearer, which will help with the upcoming dynamic attach for QUIC (which requires deferring ticket processing & 0RTT delivery until after user code). As part of this, this changes the internal flow of how ALPN & SNI are resolved. Instead of calculating them synchronously within the OpenSSL callbacks, we precalculate them at the pause, and then the OpenSSL callbacks just return the precalculated values. This is intended to only create visible changes to node:quic. Though it touches node:tls, it preserves all existing behaviour (including callback order and other details). Signed-off-by: Tim Perry <pimterry@gmail.com>
|
Review requested:
|
nodejs-github-bot
added
c++
Codecov Reportβ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #65522 +/- ##
==========================================
- Coverage 90.14% 90.11% -0.03%
==========================================
Files 751 753 +2
Lines 253585 253733 +148
Branches 47772 47818 +46
==========================================
+ Hits 228596 228661 +65
- Misses 16228 16308 +80
- Partials 8761 8764 +3
π New features to boost your workflow:
|
|
Rather than focusing the PR description on the benefits, can you explain what the behavioral / structural differences here are? |
|
Ok, I had an agent summarize out the actual changes here. Let me step through them. The quoted bits here are from the agents summary:
I know this is the key goal here with your eventual dynamic attach goal.. essentially pause here and let the JS side set things up. My goal has been to defer calling out to JS until we absolutely need to, handling as much of the handshake as possible in C++ as we can. I'm still not entirely sold on dynamic attach yet. We know at this point that the ALPN is either h3 or something else, and we know that h3 maps to Http3Application and something else maps to the default behavior. Why call out to JavaScript to have it select and install the application? That seems entirely unnecessary and not a performance win. But I guess that's an argument for the larger PR HTTP3/QUIC split PR.
+1 ... this has been on my list to revisit for a while.
Seems fine. We'll need to revisit keylog/qlog when all this is done anyway to ensure we're capturing everything correctly.
+1
This is good. Absent/malformed application data should mean that the ticket is ignored and a renewed ticket is returned. If we just ignore, the client may continue to just keep trying to use the bad ticket.
This one I'm not convinced about. Currently we will fail the handshake if an ALPN that the server does not recognize is presented. With this, we proceed with the handshake and likely fail later. I'd think that's a regression.
+1
Hmm.. not sure about this one. Smells bad.
+1
This is a regression. The stats should be accurate at this point. Beyond that, I think this PR could definitely have been better split into multiple commits but ok. |
|
Overall, +1 but there are a few regressions to investigate. |
|
Code updated. I've replied on the inline points above, but the other remaining notes from your larger comment:
I think there's a real issue here, but your description is a misreading of the summary. The problem being highlighted is only reachable when the server is explicitly configured with no ALPN protocols ( I've now tighten this up, with a validation to server creation, to just reject that empty ALPN list at source instead so this is unreachable. I think ALPN is now correctly strict everywhere for QUIC.
Yes, that was a bug. Server name handling now distinguishes the three states (valid SNI/no SNI/malformed SNI) and handles them correctly (do SNI/use wildcard SNI/fail handshake).
Agreed, now fixed. |
|
I've now added some proper benchmarks, one for pure connection setup (separately for QUIC vs H3) and one for single HTTP request time (separately for 1RTT & 0RTT). Results on my machine say no significant changes: |
3 participants
With this change, the TLS suspend mechanism I introduced in #64827 becomes generic and we reuse it in QUIC. This adds new client hello structures, which hide the OpenSSL vs BoringSSL differences, and expose an interface for each handshake flow to continue, suspend or fail the handshake. While suspended they can access the details and validate/prepare according to their needs.
There's one immediate advantage here: this allows QUIC to reuse this existing mechanism to largely replace its event deferral system. In QUIC, we need to defer some handshake events (mostly relating to resumption & 0RTT, see #64132) until the handshake is sufficiently complete so that at the
sessionevent we can do some setup with the data already available, but before the actual data processing within starts.To do this currently we have a whole event deferral mechanism and multi-step session ticket extraction & validation flow. Directly suspending the handshake reusing the existing mechanism from node:tls lets us delete that completely, do the simple thing instead, and get the same result.
This then has another medium-term benefit: as part of #63995 we will be adding dynamic attach, where the application protocol is decided dynamically on session creation, which requires deferring session ticket processing even further (because we don't even know which protocol to validate for). This mechanism gives us the structure in which to easily do that, giving us 0RTT + dynamic attach.
As part of this, this changes the internal flow of how ALPN & SNI are resolved for QUIC, and extracts ALPN helpers to share more logic between TLS/QUIC/DTLS. Instead of calculating them synchronously within the OpenSSL callbacks, we now precalculate them at the suspend point, and then the OpenSSL callbacks later just use the precalculated values.
This is intended to only create visible changes to node:quic. Though it touches node:tls & node:dtls, it's just commonizing logic and preserves all existing behaviour (including callback order and other details). In future I'd like to make some small breaking changes to both to bring these further inline, but that's tricky and we can ignore it for now.